Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use styled components #30

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Use styled components #30

wants to merge 5 commits into from

Conversation

tomatrow
Copy link
Contributor

@tomatrow tomatrow commented Oct 9, 2024

No description provided.

Copy link
Contributor

@samholmes samholmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it this PR is a draft, but nonetheless here's some feedback

Comment on lines +6 to +9
const LogoImage = styled.img`
border-radius: 0.25rem;
width: 3rem;
`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put all local components below the exported. Priority should be:

  1. Metadata stuff (interfaces/types, config, or constants)
  2. Public stuff (exports)
  3. Private stuff (local stuff like components or utilities).

const LogoImage = styled.img`
border-radius: 0.25rem;
width: 3rem;
`

const Header = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like export const instead of export default. It's clearer and allows you to export types an other things that a module may want to export.

Comment on lines 16 to 17
background: #1a1e23;
border: 1px solid #1a1e23;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a border needed if it matches the background color?

@@ -8,6 +8,34 @@ const LogoImage = styled.img`
width: 3rem;
`

const Anchor = styled.a`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move below export. Also the name should be specific to the component's purpose. Isn't this a call-to-action button? Why not name it CtaButton?

display: inline-flex;
justifiy-content: center;
justify-content: center;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixup the previous commit for the typo. Should we consider using JS instead of the string template in order for type checking to help us with typos?

Comment on lines +30 to +52
${(props) => {
const backgroundColor = props.ghost ? "transparent" : "#1a1e23"

return css`
background: ${backgroundColor};
border: 1px solid ${backgroundColor};
`
}}

&:hover {
${(props) => {
if (props.ghost)
return css`
background: color-mix(in srgb, white 5%, transparent 100%);
border: 1px solid transparent;
`
else
return css`
background: #15181d;
border: 1px solid #15181d;
`
}}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all becomes simpler with no border.

background: #15181d;
border: 1px solid #15181d;
background: color-mix(in srgb, white 5%, transparent 100%);
border: 1px solid transparent;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the border is not using the same color function for its tweening? But perhaps the border should be removed anyway, but if not then this question is relevant.


const LogoImage = styled.img`
border-radius: 0.25rem;
width: 3rem;
`

const Anchor = styled.a`
const Anchor = styled.a<{
ghost?: boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ghost isn't used. Why is it added in this commit?

@@ -52,6 +52,54 @@ const Anchor = styled.a<{
}
`

const Button = styled.a<{
ghost?: boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ghost is used everywhere or nowhere, then why is it even a parameter?

@tomatrow tomatrow marked this pull request as draft October 30, 2024 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants